Simplify and correct ProbabilitySampler behaviour #111
Conversation
fbogsany
requested review from
bai,
luvtechno,
mwear and
SergeyKanzhelev
as code owners
| apply_to_root_spans: true, | ||
| apply_to_remote_parent: true, | ||
| apply_to_all_spans: false) | ||
| apply_probability_to: :root_spans_and_remote_parent) |
There was a problem hiding this comment.
The collection of boolean flags was confusing, interdependent, incorrectly implemented, and didn't capture either the spirit or intent of the OTEP. Since we're in Ruby, we can have Symbols, so this uses a single Symbol argument to distinguish the 3 supported cases.
| @@ -26,51 +30,48 @@ def initialize(probability, hints:, ignore_parent:, apply_to_root_spans:, apply_ | |||
| # | |||
| # Callable interface for probability sampler. See {Samplers}. | |||
| def call(trace_id:, span_id:, parent_context:, hint:, links:, name:, kind:, attributes:) | |||
| take_hint(hint) || | |||
| use_parent_sampling(parent_context) || | |||
| use_link_sampling(links) || | |||
There was a problem hiding this comment.
The OTEP talks about links, but the decision table for ProbabilitySampler doesn't use them.
| use_probability_sampling(trace_id) || | ||
| NOT_RECORD | ||
| end | ||
| ignore(links, name, kind, attributes) |
There was a problem hiding this comment.
I decided to explicitly ignore the parameters the ProbabilitySampler doesn't use in its decision. It seemed cleaner that just encoding it in a comment.
| NOT_RECORD if !@apply_to_root_spans && parent_context.nil? | ||
| def sample(hint, trace_id, parent_context) | ||
| if parent_context.nil? | ||
| hint == HINT_RECORD_AND_PROPAGATE || probably(trace_id) |
| if parent_context.nil? | ||
| hint == HINT_RECORD_AND_PROPAGATE || probably(trace_id) | ||
| else | ||
| parent_sampled_flag(parent_context) || hint == HINT_RECORD_AND_PROPAGATE || probably_for_child(parent_context, trace_id) |
| @@ -60,12 +60,6 @@ | |||
| result.wont_be :sampled? | |||
| end | |||
|
|
|||
| it 'respects link sampling' do | |||
There was a problem hiding this comment.
ProbabilitySampler doesn't respect Link sampling.
| @@ -83,35 +77,30 @@ | |||
| result.wont_be :sampled? | |||
| end | |||
|
|
|||
| it 'does not sample a root span unless apply_to_root_spans' do | |||
There was a problem hiding this comment.
This is not a supported case for ProbabilitySampler. The probability applies to "root spans", "root spans and remote parent", or "all spans".
sdk/lib/opentelemetry/sdk/trace/samplers/probability_sampler.rb
Outdated
Show resolved
Hide resolved
sdk/lib/opentelemetry/sdk/trace/samplers/probability_sampler.rb
Outdated
Show resolved
Hide resolved
sdk/lib/opentelemetry/sdk/trace/samplers/probability_sampler.rb
Outdated
Show resolved
Hide resolved
sdk/lib/opentelemetry/sdk/trace/samplers/probability_sampler.rb
Outdated
Show resolved
Hide resolved
|
This is ready for another 👀 |
This simplifies the interface and implementation of the
ProbabilitySampler, as well as fixing some bugs wrt the OTEP: https://github.com/open-telemetry/oteps/blob/master/text/0006-sampling.mdI'll call out specifics in comments.